-
Notifications
You must be signed in to change notification settings - Fork 53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add progress trackers during test execution #205
Conversation
This looks really cool @JoshKarpel! One thing I did notice though is that it seems to roughly double the amount of time taken for the Ward test suite to run (I think output to the terminal is actually where Ward spends most of it's time at the moment). With the slowdown and flickering issues that seem to happen in some environments, maybe we're best putting it behind an option like I quite like the idea of having On my laptop the Ward test suite runs in ~0.53s without the progress bar and ~1.04s with the progress bar. Running Also, I didn't notice any flickering on my side (using iTerm2 on MacOS). |
Agreed. Latest commit has This might bite back later if we end up with lots of styles that are incompatible with each other, but in the meantime I like having both going at once...
Same for me, but worse :(
Probably related to my flickering issues - seems like Windows Terminal is pretty slow. I don't love how I'm passing information around right now. Some functions that were previously pretty unaware of higher-level information (configuration and test suite state) now need to know much more. I'm considering trying to roll some of this behavior back up into the |
Codecov Report
@@ Coverage Diff @@
## master #205 +/- ##
==========================================
- Coverage 75.62% 74.54% -1.09%
==========================================
Files 17 17
Lines 1522 1579 +57
Branches 255 266 +11
==========================================
+ Hits 1151 1177 +26
- Misses 334 362 +28
- Partials 37 40 +3
Continue to review full report at Codecov.
|
@darrenburns Well, I thought about it and didn't figure out anything clever. For the moment this API is totally internal, so I suppose it doesn't matter much. I could imagine wanting to let people register new output styles, at which point the interface would need to get nailed down and (hopefully) shrunk. Passing the Let me know what you think - try to solve in this PR, or move forward with something like this and open a discussion on customizable test output? Something else? |
@JoshKarpel I've put a couple of comments on the PR, I know you still have it marked as a draft, so haven't went too deep. |
Ok, I think this is ready for a closer look. Codecov isn't very happy with me - I added some tests of the CLI that hit the front end, but don't end up executing the full suite. This code is pretty deep and I'm not sure how to get at it to test in a structured way. I think we could get a cheap coverage win by writing out a basic suite to a file and executing it using the click test runner, but I don't know how valuable those tests would really be 🤷🏻♂️ . Something to consider for an eventual |
Ack, this fell off my plate and I lost track of it... @darrenburns can you take another look when you get a chance? |
Sure thing, will try and have a look at the weekend |
That was a fun one! Ended up being a one line fix here: 7be59ca#diff-e0598ee7e5ba2e5d5466f9da72e68c3bcd19ccaed119f39beae9b66a829cad4dR351 but I tweaked a few other things while debugging and decided to keep them. |
Codecov Report
@@ Coverage Diff @@
## master #205 +/- ##
==========================================
- Coverage 75.62% 74.46% -1.17%
==========================================
Files 17 17
Lines 1522 1582 +60
Branches 255 266 +11
==========================================
+ Hits 1151 1178 +27
- Misses 334 364 +30
- Partials 37 40 +3
Continue to review full report at Codecov.
|
…ble-printing end-of-line when the number of tests is equal to the max number of dots for the line
@darrenburns looks like an ephemeral error on one of the test runs; can you rerun CI? How strongly do you feel about the coverage checks? I'm really not sure how to test these beyond "make sure it runs", which I suppose has some value. Would involve having a part of the test suite that writes a dummy test suite to a temp file and runs the CLI over it, I think. |
@JoshKarpel Unfortunately those errors happen quite frequently in CI and there's no way to rerun a single part of a matrix-based workflow :( I've ran it again but looks like it's failed on a different version/OS combo this time. I'm not precious about the coverage for terminal output stuff at the moment, it's an area that's always been in flux and hard to test. As part of adding plugin/hook support in #210 I'll probably be looking to rewrite a lot of |
:(
Sounds like a plan! Anything blocking merge? Sounds like it will unblock some other work in the near future. |
I just need to add some docs (there's a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry! Thought I'd 👍 'd this earlier
Oo, hadn't noticed this! Love it. Writing docs is tricky, and I don't want to mess with your style (or mix in screenshots that look inconsistent!), so I I think I'll let you do it, if you don't mind :) |
Released as |
My pleasure! :D |
Here's a take on https://github.com/darrenburns/ward/discussions/197 , using a Rich progress bar displayed below the running tests.
Putting this up for an early look (@darrenburns) because there are a few problems with it that might need discussion...
dots
output displays, because it seems likeProgress
is expecting you to not print withend=""
while it's running. Perhaps we can make some improvements to Rich itself to make that work better.